Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add CanEqual typeclass instances of Option, Either and Tuple for strictEquality #12419

Conversation

kevin-lee
Copy link
Contributor

I'm sorry to create this PR without prior discussion but today is the 11th of May (in Sydney Australia) and the latest Scala 3 blog post says

"3.0.0 stable will get a green light if no critical bugs are discovered until May 12th."

I hope this PR is considered before the stable release and I thought that it would be good to discuss it here if there is any chance that it can be merged before that day.

Add CanEqual typeclass instances of Option, Either and Tuple for strictEquality

Additional details:

  • In addition to CanEqual[Option[T], Option[U]], CanEqual[Option[T], Option[T]] is also added for None case in pattern matching.
  • CanEqual instances for Tuple are added to Tuple companion object since Tuple is Scala 3's new type unlike Option and Either.

Motivation and Reason

I recently started using Scala 3 for my personal projects and as I use more, I found strictEquality really useful and it makes my code much safer. In fact, I found this bug in Scala 3 I reported and fixed with strictEquality.

However, as soon as I enable strictEquality, it is so inconvenient and difficult to use very common Scala types like Option, Either and Tuple.
For instance, the following code doesn't compile because None and Option[Int] cannot be compared with == or !=

val option1: Option[Int] = Some(1)
option1 match {
  case Some(n) =>
    println(s"${n.toString}")
  case None =>
    // error: Values of types object None and Option[Int] cannot be compared with == or !=
    println("None")
}

and there are more

(1, "a") == (1, "a")
// error: Values of types (Int, String) and (Int, String) cannot be compared with == or !=

val option1: Option[Int] = Some(1)
option1 == Some(1)
// error: Values of types Option[Int] and Some[Int] cannot be compared with == or !=
33

val either1: Either[String, Int] = Right(1)
val either2: Either[String, Int] = Right(1)
either1 == either2
// error: Values of types Either[String, Int] and Either[String, Int] cannot be compared with == or !=

Also please check out the tests I added in this PR.

Of course, we can create custom CanEqual typeclass instances for Option, Either and Tuple but then these types are from Scala so we can't put them in their companion objects meaning we have to import the custom ones whenever we want to check equality of those types. Checking their equality is so common case but it's so inconvenient to do without the typeclass instances. That's why I'm proposing these typeclass instance.

What about Try?

I could not add CanEqual typeclass instance for Try because Try has no type parameter for Failure case which contains Throwable, so it is hard to make the following case fail in compile-time.

val try1: Try[Int] = Failure(new Exception("error"))
val try2: Try[Int] = Failure(new RuntimeException("error"))
try1 == try2 // This should cause a compile-time error but it doesn't!

// The above case should fail because the following one fails.
new Exception("error") == new RuntimeException("error") // compile-time error

@kevin-lee kevin-lee changed the title Add CanEqual typeclass instances for Option, Either and Tuple for strictEquality Add CanEqual typeclass instances of Option, Either and Tuple for strictEquality May 11, 2021
@kevin-lee kevin-lee force-pushed the add-CanEqual-typeclass-instances-for-Option-Either-Tuple branch from 230fa78 to bd62ef1 Compare May 11, 2021 13:10

// The next three typeclass instances can also go into the companion objects of classes Option and Either.
// For now they are here in order not to have to touch the source code of these classes
given canEqualOptions[T, U](using eq: CanEqual[T, U]): CanEqual[Option[T], Option[U]] = derived
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this can only be added in 3.1. Not sure if we can use @experimnetal here because the users will no have the choice to use it or not.

@sjrd @smarter WDYT?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed. This must be for 3.1.

Copy link
Contributor Author

@kevin-lee kevin-lee May 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @nicolasstucki and @sjrd for your answers. I have a few questions. I'm so sorry to ask it when everyone is busy with finalizing Scala 3 for release and I really appreciate your answer.

  1. Regarding the users having no choice once it's added, could you please think about who are affected by this?
    They are the ones who actually need strictEquality and most likely need this typeclass instance. The ones don't use strictEquality are not affected at all. Besides, the users do still have contol of CanEqual[T, U] since it's not auto-magically give them CanEqual[T, U] as well.

    Can there be another use case than this one? I think it would be hard to find any other use case of CanEqual for Option but I know that I could be easily wrong so I'm happy to take any advice.

  2. If what I mentioned above is not so important, that's fine. Could you tell me about the rest please?
    a. Can we have the other CanEqual for Option to solve cannot be compared with == or != on None case in pattern matching?
    b. Can we have CanEqual for Either?
    c. Can we have CanEqual for Tuple?

It will be so nice if at least the ones for Tuple are accepted for 3.0 because it's so common use case.

Finally, could you please think about what would make strictEquality usable and more useful. Whenever users use Option, Either and Tuple, they need to import their own CanEqual typeclass instances seprately and it would be so annoying.

As far as I know, wildcard import like import my.typeclasses._ doesn't work and it has to be done seprately like.

import my.typeclasses.{canEqualOptions, canEqualOption, canEqualEither, canEqualEmptyTuple, canEqualTuple}

Thank you!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I just found that 3.0.0 has been tagged already. 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding,

As far as I know, wildcard import like import my.typeclasses._ doesn't work and it has to be done seprately like.

I didn't know that I could do import my.typeclasses.given for that. Sorry about lack of my Scala 3 knowledge. 😅

@nicolasstucki nicolasstucki added this to the 3.1.0 milestone May 12, 2021
@kevin-lee
Copy link
Contributor Author

Oh I misunderstood about Scala 3.0.0. I didn't know that it wouldn't contain the most recent commits. So it doesn't look like there's any way that this PR can be included in 3.0.0. 😅 My bad.

Do you think I should create a separate PR for just Tuple? Just in case, CanEqual for Tuple might have more chance to be accepted before the other ones?

@smarter smarter marked this pull request as draft May 17, 2021 15:14
Copy link
Member

@smarter smarter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Otherwise LGTM for 3.1.

library/src/scala/CanEqual.scala Outdated Show resolved Hide resolved
@kevin-lee kevin-lee force-pushed the add-CanEqual-typeclass-instances-for-Option-Either-Tuple branch from bd62ef1 to 18971cb Compare May 17, 2021 15:28
@smarter smarter added the needs-minor-release This PR cannot be merged until the next minor release label May 17, 2021
@kevin-lee kevin-lee force-pushed the add-CanEqual-typeclass-instances-for-Option-Either-Tuple branch from 18971cb to 77fc62f Compare May 17, 2021 15:41
@bishabosha
Copy link
Member

Hi @kevin-lee, would you be able to maybe rebase and add MIMA exclusions in https://github.com/lampepfl/dotty/blob/master/project/MiMaFilters.scala?

@smarter
Copy link
Member

smarter commented Jul 8, 2021

@bishabosha I don't think adding mima exclusion makes sense here, we can only merge this PR once we've bumped our base version to 3.1.0-RC1, and at that point mima will not error out on new definitions.

@bishabosha
Copy link
Member

@bishabosha I don't think adding mima exclusion makes sense here, we can only merge this PR once we've bumped our base version to 3.1.0-RC1, and at that point mima will not error out on new definitions.

Ah ok, thank you for explaining

@kevin-lee
Copy link
Contributor Author

Whenever I need to rebase, please let me know. 🙂

@bbarker
Copy link
Contributor

bbarker commented Aug 5, 2021

I recently came across this case:

val ints = List(1,2,3)

ints match {
  case x::_ => println(x)
  case (Nil: List[Int]) => println("it's empty")
}

I would suggest adding a test case for this, as it does not compile on Scala 3.0.1 with -language:strictEquality enabled (I tried adding the explicit type annotation to Nil, but this didn't change the result). Maybe this is out of scope for the PR, but just in case, I'll mention it here.

Edit:

Here's the error:

Values of types object scala.collection.immutable.Nil and List[Int] cannot be compared with == or !=.
I found:

    CanEqual.canEqualSeq[T, U](/* missing */summon[CanEqual[?, ? >: Int]])

But no implicit values were found that match type CanEqual[?, ? >: Int].

@kevin-lee
Copy link
Contributor Author

@bbarker Thanks Brando! I'm glad that you pointed it out. I've been trying to figure out what common CanEqual cases we usually have in practice so created this and have been using it in my projects and discovered the issue you mentioned as well. So I added it here which fixes it. I'll update this PR or create another PR with that.

@smarter Could you please tell me if I should add that case to this PR or create another one?

@bbarker
Copy link
Contributor

bbarker commented Aug 6, 2021

Hi @kevin-lee - thanks for pointing out the repository. It did seem to address the toy example above. Unfortunately, the example that inspired it still had the same issue. So, I tried adding an additional layer of indirection (Option), and this did not have an issue. But when I finally used my custom data type, that's where I could see the issue:

    val intOpts: List[Option[Int]] = List(Some(1), None, Some(3))
    intOpts match {
      case x::_ => println(x)
      case Nil => println("it's empty")
    }

    val intIOs: List[MyIO[Any, Unit, Int]] = List(
      MyIO.pure(1), MyIO.fail(()), MyIO.pure(3)
    )
    intIOs match {
      case x::_ => println(x)
      case Nil => println("it's empty")
    }

The error:

[error] -- Error: /Users/bbarker/workspace/zionomicon-exs/src/main/scala/chapter2/Chapter2Exs.scala:214:11 
[error] 214 |      case Nil => println("it's empty")
[error]     |           ^^^
[error]     |Values of types object scala.collection.immutable.Nil and List[chapter2.Chapter2Exs.MyIO[Any, Unit, Int]] cannot be compared with == or !=.
[error]     |I found:
[error]     |
[error]     |    CanEqual.canEqualSeq[T, U](
[error]     |      /* missing */
[error]     |        summon[CanEqual[?, ? >: chapter2.Chapter2Exs.MyIO[Any, Unit, Int]]]
[error]     |    )
[error]     |
[error]     |But no implicit values were found that match type CanEqual[?, ? >: chapter2.Chapter2Exs.MyIO[Any, Unit, Int]].

I guess the issue is that we don't have a given for my custom type, so that probably explains it. My only question: is this a circumstance where I should expect to implement the given myself, or is there a better way?

Also, a related issue occurs with None and Option, and no custom data type is involved:

[error] 22 |        case None      => cause
[error]    |             ^^^^
[error]    |Values of types object None and Option[Throwable] cannot be compared with == or !=.
[error]    |I found:

@kevin-lee
Copy link
Contributor Author

kevin-lee commented Aug 6, 2021

@bbarker Based on what you said, I guess your MyIO type doesn't have CanEqual typeclass instance. Yes, you do need it.
Could you please derives CanEqual?
e.g.)

final case class MyIO[F[_], A, B](...) derives CanEqual
// or
class MyIO[F[_], A, B](...) derives CanEqual

or if the first param is not a higher-kinded type like F instead of F[_] and you want to put something like Any for that as your example shows.

final case class MyIO[F, A, B](...) derives CanEqual

@kevin-lee
Copy link
Contributor Author

@bbarker Please have a look at https://scastie.scala-lang.org/Kevin-Lee/ThWpw5D9R1qVDeAX0zUq6Q
NOTE: It's using the can-equal library I mentioned above.

@smarter
Copy link
Member

smarter commented Aug 6, 2021

@smarter Could you please tell me if I should add that case to this PR or create another one?

I would say make a new one since this one is already approved and ready to go once the version is bumped while anything new would have to be properly reviewed first.

@bbarker
Copy link
Contributor

bbarker commented Aug 6, 2021

Thanks @kevin-lee - this is pretty neat, I didn't know about derives (in Scala 3). But the plot thickens further. This does fix the issue I had above, but once again, my real case on my canEqual branch has issues 😆 :

[error] -- Error: /Users/bbarker/workspace/zionomicon-exs/src/main/scala/chapter2/Chapter2Exs.scala:229:15 
[error] 229 |          case Nil => Right(builder)
[error]     |               ^^^
[error]     |Values of types object scala.collection.immutable.Nil and List[chapter2.Chapter2Exs.MyIO[R, E, A]] cannot be compared with == or !=.
[error]     |I found:
[error]     |
[error]     |    CanEqual.canEqualSeq[T, U](
[error]     |      chapter2.Chapter2Exs.MyIO.derived$CanEqual[R_$_L, R_$_R, E_$_L, E_$_R, A_$_L, 
[error]     |        A_$_R
[error]     |      ](
[error]     |        chapter2.Chapter2Exs.MyIO.derived$CanEqual[Nothing, Nothing, EmptyTuple.type
[error]     |          , 
[error]     |        EmptyTuple.type, EmptyTuple.type, EmptyTuple.type](
[error]     |          canequal.all.canEqualEmptyTuple
[error]     |        , canequal.all.canEqualEmptyTuple, canequal.all.canEqualEmptyTuple)
[error]     |      , /* missing */summon[CanEqual[?, ? >: E]], ???)
[error]     |    )
[error]     |
[error]     |But no implicit values were found that match type CanEqual[?, ? >: E].

@kevin-lee
Copy link
Contributor Author

I would say make a new one since this one is already approved and ready to go once the version is bumped while anything new would have to be properly reviewed first.

@smarter Ok, make sense. I will create a new one for that. Thank you!

@kevin-lee
Copy link
Contributor Author

@bbarker Please try it with can-equal
In build.sbt,

libraryDependencies += "io.kevinlee" %% "can-equal" % "0.1.0"

and

import canequal.all.given

It looks fine.
https://scastie.scala-lang.org/Kevin-Lee/ThWpw5D9R1qVDeAX0zUq6Q/2

@kevin-lee
Copy link
Contributor Author

@bbarker Sorry, I think I accidentally removed -language:strictEquality. Let me check it again.

@kevin-lee
Copy link
Contributor Author

@bbarker In your case having

given canEqualMyIO[R, E, A]: CanEqual[MyIO[R, E, A], MyIO[R, E, A]] = CanEqual.derived

inside the MyIO companion object solves the pattern matching issue, but The actual equality check for such type is not working since it contains a function in it although it's a case class.
So

MyIO.pure(1) == MyIO.pure(1)

is alway false.
https://scastie.scala-lang.org/Kevin-Lee/ThWpw5D9R1qVDeAX0zUq6Q/5

@bbarker
Copy link
Contributor

bbarker commented Aug 8, 2021

@bbarker In your case having

given canEqualMyIO[R, E, A]: CanEqual[MyIO[R, E, A], MyIO[R, E, A]] = CanEqual.derived

inside the MyIO companion object solves the pattern matching issue, but The actual equality check for such type is not working since it contains a function in it although it's a case class.
So

MyIO.pure(1) == MyIO.pure(1)

is alway false.
https://scastie.scala-lang.org/Kevin-Lee/ThWpw5D9R1qVDeAX0zUq6Q/5

Ah, makes sense for sure. Now, I do wonder if it makes sense that we have to have CanEqual givens for a type parameter when we are only pattern matching on the constructors of an ADT (in this case, List). I think @smarter suggested that might be a dotty bug - is it worth filing a separate issue for that?

@bbarker
Copy link
Contributor

bbarker commented Aug 12, 2021

@kevin-lee Would it make sense to add given CanEqual[None.type, Option[?]] = CanEqual.derived ? Maybe some related givens for Nil (though I've not tried to see if that works yet).

- In addition to CanEqual[Option[T], Option[U]], CanEqual[Option[T], Option[T]] is also added for None case in pattern matching.
- CanEqual instances for Tuple are added to Tuple companion object since Tuple is Scala 3's new type unlike Option and Either.
@dwijnand dwijnand force-pushed the add-CanEqual-typeclass-instances-for-Option-Either-Tuple branch from 77fc62f to 3e963c0 Compare August 19, 2021 07:42
@dwijnand dwijnand marked this pull request as ready for review August 19, 2021 07:42
@dwijnand dwijnand merged commit 4fb0a61 into scala:master Aug 19, 2021
@dwijnand
Copy link
Member

Doesn't the need for these instances go away if we can summon the trivial CanEqual[A, Nothing] and CanEqual[Nothing, B]?

@bbarker
Copy link
Contributor

bbarker commented Aug 23, 2021

I may be misunderstanding, but it seems like CanEqual[A, Nothing] is an escape-hatch that works for anything, and is thus not something you'd want to have floating around file-wide scope. Conversely, I can't think of any issues that might arise from using CanEqual[None.type, Option[?]] anywhere.

@kevin-lee kevin-lee deleted the add-CanEqual-typeclass-instances-for-Option-Either-Tuple branch August 24, 2021 08:53
@kevin-lee
Copy link
Contributor Author

@dwijnand Let me try to find if there's a better way or any issue with CanEqual[A, Nothing] and CanEqual[Nothing, B] in practice. Now my personal projects are all Scala 3 projects and I always turn on strictEquality so will see how it goes. Thanks!

@kevin-lee
Copy link
Contributor Author

@bbarker My initial solution was something similar to what you asked. It was like

given optionCanEqual[A]: CanEqual[None.type, Option[A]] = CanEqual.derived

However it's not commutative and it doesn't make sense if equality is not commutative.

None == Some(1) // fine
Some(1) == None // compile-time error

So I had to add two typeclass instances like

given noneOptionCanEqual[A]: CanEqual[None.type, Option[A]] = CanEqual.derived
given optionNoneCanEqual[A]: CanEqual[Option[A], None.type] = CanEqual.derived

and that's why I eventually came up with a simpler solution which was just

given canEqualOption[T](using eq: CanEqual[T, T]): CanEqual[Option[T], Option[T]] = CanEqual.derived

If your concern is about pattern matching on List containing your MyIO, I wouldn't worry too much about it and think about alternative way to iterate.
Because, in practice, you wouldn't check the equality of an effect but what it is evaluated to. Besides, you wouldn't probably do pattern matching on some List containing effects. In practice, you would probably do one of these

  • use flatMap
  • for-comprehension
  • map and sequence
  • traverse
    In most cases when I have List[F[A]], I just use traverse.

If you really want to pattern match on List containing MyIO to iterate but don't want to have compile-time error on Nil case when strictEquality is on, and you don't like to have CanEqual for MyIO because it doesn't look like it makes sense then you can simply replace Nil with List(). With List(), you don't need to have CanEqual.

e.g.) https://scastie.scala-lang.org/Kevin-Lee/1qcXHY8DR3Gy2FeCK9F7hQ

@bbarker
Copy link
Contributor

bbarker commented Aug 30, 2021

First, sorry for the delayed response, and thank you for the detailed reply.

@bbarker My initial solution was something similar to what you asked. It was like

given optionCanEqual[A]: CanEqual[None.type, Option[A]] = CanEqual.derived

However it's not commutative and it doesn't make sense if equality is not commutative.

None == Some(1) // fine
Some(1) == None // compile-time error

Interesting, I didn't realize it wasn't commutative. Naively I would think equality should be made commutative in a PL whenever possible, but I suppose the workaround is easy enough as you showed below:

So I had to add two typeclass instances like

given noneOptionCanEqual[A]: CanEqual[None.type, Option[A]] = CanEqual.derived
given optionNoneCanEqual[A]: CanEqual[Option[A], None.type] = CanEqual.derived

and that's why I eventually came up with a simpler solution which was just

given canEqualOption[T](using eq: CanEqual[T, T]): CanEqual[Option[T], Option[T]] = CanEqual.derived

Hmm, this doesn't seem to work:

package chapter4
import zio.*
import canequal.all.given

object Chapter4Exs:

  def failWithMessageCaught(string: String): Task[Nothing] =
    failWithMessageOrig(string).sandbox.mapError { cause =>
      //given CanEqual[None.type, Option[?]] = CanEqual.derived // TODO - see about adding this to Scala prelude
      given canEqualOption[T](using eq: CanEqual[T, T]): CanEqual[Option[T], Option[T]] = CanEqual.derived
      cause.dieOption match {
        case Some(thr) => Cause.Fail(thr)
        case None      => cause
      }
    }.unsandbox

This results in:

[error] -- Error: /Users/bbarker/workspace/zionomicon-exs/src/main/scala/chapter4/Chapter4Exs.scala:24:13 
[error] 24 |        case None      => cause
[error]    |             ^^^^
[error]    |Values of types object None and Option[Throwable] cannot be compared with == or !=.
[error]    |I found:
[error]    |
[error]    |    canequal.all.canEqualOptions[T, U](
[error]    |      /* missing */summon[CanEqual[?, ? >: Throwable]]
[error]    |    )
[error]    |
[error]    |But no implicit values were found that match type CanEqual[?, ? >: Throwable].
[error]    |

So far, I've not thought of any single-given solutions to this issue that don't lose type safety, so maybe going back to the pair of symmetric givens above is the better approach?

If your concern is about pattern matching on List containing your MyIO, I wouldn't worry too much about it and think about alternative way to iterate.
Because, in practice, you wouldn't check the equality of an effect but what it is evaluated to. Besides, you wouldn't probably do pattern matching on some List containing effects. In practice, you would probably do one of these

  • use flatMap
  • for-comprehension
  • map and sequence
  • traverse
    In most cases when I have List[F[A]], I just use traverse.

If you really want to pattern match on List containing MyIO to iterate but don't want to have compile-time error on Nil case when strictEquality is on, and you don't like to have CanEqual for MyIO because it doesn't look like it makes sense then you can simply replace Nil with List(). With List(), you don't need to have CanEqual.

e.g.) https://scastie.scala-lang.org/Kevin-Lee/1qcXHY8DR3Gy2FeCK9F7hQ

This is a very good point - my rustiness with Scala is showing; I'll try to remember to use List() instead going forward.

@kevin-lee
Copy link
Contributor Author

Doesn't the need for these instances go away if we can summon the trivial CanEqual[A, Nothing] and CanEqual[Nothing, B]?

@dwijnand I've done some testing and it doesn't work.

import scala.language.strictEquality

given nothingCanEqual1[A]: CanEqual[A, Nothing] = CanEqual.derived
given nothingCanEqual2[A]: CanEqual[Nothing, A] = CanEqual.derived

val optionalValue: Option[String] = Some("A")

optionalValue match {
  case Some(value) => println(s"value: $value")
  case None => println("None")
}
-- Error:
    case None => println("None")
         ^^^^
Values of types object None and Option[String] cannot be compared with == or !=

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-minor-release This PR cannot be merged until the next minor release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants